🦋 Changeset detected

Latest commit: 71fbcbd

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Maybe we should get rid of the try-catch entirely, and instead do more introspection on the passed value instead, e.g. "is this falsy or a function".

I think that's directionally correct, yeah — the try-catch is messy and brittle. It's not quite as simple as just doing this though...

export function validate_dynamic_component(component_fn) {
  if (component_fn !== undefined && typeof component_fn !== 'function') {
    e.svelte_component_invalid_this_value();
  }

  const instance = component_fn();

  if (instance !== undefined && typeof instance !== 'object') {
    e.svelte_component_invalid_this_value();
  }

  return instance;
}

...because component_fn is a () => $.validate_component($$component, ...) thunk. So the argument to validate_dynamic_component will always be a function, even if $$component isn't

Could it be worth adding a component symbol too?

That wouldn't help here, because $$component isn't the thing being validated. Not sure what the most elegant fix looks like

That wouldn't help here, because $$component isn't the thing being validated. Not sure what the most elegant fix looks like

But validate_component still returns the component function...we need to do the check on the returned value rather than on the fn itself. It's wrong?

The root of the issue is that validate_dynamic_component can't 'see' $$component; it gets passed a thunk instead. It has direct knowledge of the return value of $$component, but no direct knowledge of $$component itself (currently, it sidesteps that by having indirect knowledge via exceptions). That's the part that would need to change

The root of the issue is that validate_dynamic_component can't 'see' $$component; it gets passed a thunk instead. It has direct knowledge of the return value of $$component, but no direct knowledge of $$component itself (currently, it sidesteps that by having indirect knowledge via exceptions). That's the part that would need to change

Ohhh yeah now I see that. I was thinking....could we move this validation to validate_component? This function will always get the thunk with validate_component and is not doing much rather than this try catch. If we remove the try catch and move the validation I'm validate_component can have access to $$component

That makes sense, yeah. It would also cover this missing validation that way. We'd need to vary the error message based on whether it's a dynamic component or not

That makes sense, yeah. It would also cover this missing validation that way. We'd need to vary the error message based on whether it's a dynamic component or not

Ok I will take a shot at it later on this branch

That makes sense, yeah. It would also cover this missing validation that way. We'd need to vary the error message based on whether it's a dynamic component or not

I think I got it mostly working. Initially I used the newly introduced ORIGINAL and FILENAME to check if it was a component. Then I refactored with a new symbol because it seemed better. However it's the playground itself is kinda failing because the root layout of sveltekit doesn't have the symbol I added (I just added it after the FILENAME add and in the hmr accept block).

I'm exploring why this is, I wonder if it's because those are being created with Svelte4Class instead.

Oh i think i got it now: the component that we need to validate is a sveltekit layout (the one from the playground) that is build and imported and so it doesn't have the symbol (since it's only added in dev).

I'm an idiot...it was a typo lol

@Rich-Harris this should be ready...everything works but i hope everything is correct enough 😄 Open to feedback obviously 🙂

I'm fine with the IS_COMPONENT approach, but I'd like to point out that it means we also need something like #11544, because else you can't decorate components without getting false positive errors later on.
Adding this symbol also means we would have a way to detect whether or not something is a component at runtime (which was trivial using instanceof SvelteComponent previously, and is impossible currently).

I'm fine with the IS_COMPONENT approach, but I'd like to point out that it means we also need something like #11544, because else you can't decorate components without getting false positive errors later on. Adding this symbol also means we would have a way to detect whether or not something is a component at runtime (which was trivial using instanceof SvelteComponent previously, and is impossible currently).

I agree, I actually thought about that. Even tho is technically possible with a proxy but it's pretty convoluted.

I'd like to point out that it means we also need something like #11544

I don't love this constraint, to be honest. I'm wondering if we can do away with the bootleg typechecking altogether and rely on actual typechecking. If we aligned the signature of server and client snippets so that snippets received thunks on the server, just like on the client — a tiny bit of overhead but it would allow people to 'decorate' snippets (we need a better name for it btw) just like they can decorate components — then we could get rid of the symbol stuff.

Basically:

export interface Component<...> {
  (
    this: void,
    internal: ComponentInternals,
    props: Props
  ): {
    // ...
  } & Exports;
  element?: typeof HTMLElement;
  z_$$bindings?: Bindings;
}

export interface Snippet<Parameters extends unknown[] = []> {
  (
    this: void,
    internal: SnippetInternals,
    ...args: number extends Parameters['length'] ? never : Getters<Parameters>
  ): typeof SnippetReturn;
}

This feels to me like a better solution than expanding the API surface area for a niche use case, and it pre-empts the question 'I can see the code that Svelte generates in the JS Output tab of the playground — why can't I add props and curry snippet arguments?'

I'd like to point out that it means we also need something like #11544

I don't love this constraint, to be honest. I'm wondering if we can do away with the bootleg typechecking altogether and rely on actual typechecking. If we aligned the signature of server and client snippets so that snippets received thunks on the server, just like on the client — a tiny bit of overhead but it would allow people to 'decorate' snippets (we need a better name for it btw) just like they can decorate components — then we could get rid of the symbol stuff.

Basically:

export interface Component<...> {
  (
    this: void,
    internal: ComponentInternals,
    props: Props
  ): {
    // ...
  } & Exports;
  element?: typeof HTMLElement;
  z_$$bindings?: Bindings;
}

export interface Snippet<Parameters extends unknown[] = []> {
  (
    this: void,
    internal: SnippetInternals,
    ...args: number extends Parameters['length'] ? never : Getters<Parameters>
  ): typeof SnippetReturn;
}

This feels to me like a better solution than expanding the API surface area for a niche use case, and it pre-empts the question 'I can see the code that Svelte generates in the JS Output tab of the playground — why can't I add props and curry snippet arguments?'

Does this mean only TS user will get those kind of warnings? So if i've interpreted this correctly you don't like the symbol idea for IS_COMPONENT right? Not a problem obviously just trying to understand if there's something else to do in this PR or we want to try to implement the TS only solution.

Does this mean only TS user will get those kind of warnings?

Yes, though that doesn't mean .ts and lang="ts", it means 'has a tsconfig.json or jsconfig.json', which we should be encouraging everyone to have. And that's fine — we don't write our add functions like this...

function add(a, b) {
  if (typeof a !== 'numner' || isNaN(a)) {
    throw new Error('first argument to `add` must be a number');
  }

  if (typeof b !== 'numner' || isNaN(a)) {
    throw new Error('second argument to `add` must be a number');
  }

  return a + b;
}

...we write them like this:

function add(a: number, b: number) {
  return a + b;
}

There's no particular reason that components and snippets should be treated differently.

So yeah, I think the solution warrants a separate PR — will try and rustle one up later if no-one beats me to it

Does this mean only TS user will get those kind of warnings?

Yes, though that doesn't mean .ts and lang="ts", it means 'has a tsconfig.json or jsconfig.json', which we should be encouraging everyone to have. And that's fine — we don't write our add functions like this...

function add(a, b) {
  if (typeof a !== 'numner' || isNaN(a)) {
    throw new Error('first argument to `add` must be a number');
  }

  if (typeof b !== 'numner' || isNaN(a)) {
    throw new Error('second argument to `add` must be a number');
  }

  return a + b;
}

...we write them like this:

function add(a: number, b: number) {
  return a + b;
}

There's no particular reason that components and snippets should be treated differently.

So yeah, I think the solution warrants a separate PR — will try and rustle one up later if no-one beats me to it

Ok...keeping this open in case we want to revisit later. This would require also a sister PR in language-tools right?

Yes

(btw you don't need to quote the entire comment you're replying to! It makes these threads longer and harder to read)

Closing in favor of #12507